Assorted usability fixes by dcrankshaw · Pull Request #192 · ucbrise/clipper

dcrankshaw · 2017-06-02T08:05:11Z

This PR makes several backwards-compatible usability fixes to make Clipper less confusing. In particular:

Fixes Remove uid from prediction queries until personalization is supported #184: Removes uid from prediction query schema which was very confusing, because the uid must be set to 0 in the current version of Clipper. Now we just set it internally.
Fixes Unloading old models #176: Adds a method remove_inactive_containers() to clean up inactive containers (@rmdort)
Addresses several of the issues in Confusing parts of Clipper Manager API #188
- imports the Clipper class in the clipper_admin module, so now users can import Clipper with from clipper_admin import Clipper
- Makes inspect_selection_policy private so it doesn't show up in API documentation
- Makes model labels optional and removes them from the examples
Adds a check to clipper_admin for python 3 and exits with an error message if Python 3 is detected.

AmplabJenkins · 2017-06-02T08:27:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/356/
Test FAILed.

Corey-Zumar · 2017-06-02T13:47:11Z

clipper_admin/__init__.py

+import sys
+if sys.version_info >= (3, 0):
+    sys.stdout.write(
+        "Sorry, clipper_admin requires Python 2.x, not Python 3.x\n")


"Sorry, clipper_admin requires Python 2.x, but you are running Python 3.x" seems clearer

Corey-Zumar · 2017-06-02T13:49:28Z

clipper_admin/clipper_manager.py


        Example
        -------
+        Define a feature function ``center()`` and train a model on the featurized input::


Nit: Extra colon after input

That's an intentional pydoc markup symbol (https://docs.python.org/devguide/documenting.html#source-code)

Got it, thanks for clarifying!

Corey-Zumar · 2017-06-02T13:52:47Z

clipper_admin/clipper_manager.py

            self.add_container(model_name, model_version)

+    def remove_inactive_containers(self, model_name):
+        # TODO: Test this function


I'm assuming this is blocked by #187 and you'd like to merge this PR first. If not, we should use the branch associated with #187 as a base and add a test.

Yeah I want to add a test for this. Let's get #187 merged before this. Leaving the TODO in for now.

Corey-Zumar · 2017-06-02T13:56:57Z

examples/tutorial/tutorial_part_two.ipynb

@@ -201,7 +199,6 @@
    "    2,\n",
    "    os.path.abspath(\"tf_cifar_model\"),\n",
    "    \"clipper/tf_cifar_container:latest\",\n",


Should the cifar and sklearn containers reference version 0.1 as opposed to latest as well?

I don't think it matters for this tutorial.

Corey-Zumar · 2017-06-02T14:08:48Z

clipper_admin/clipper_manager.py

+            for container in container_ids:
+                # returns a string formatted as "<model_name>:<model_version>"
+                container_model_name_and_version = self._execute_root(
+                    "docker inspect --format \"{{ index .Config.Labels \\\"%s\\\"}}\" %s"


This fails in the case where no model containers are actively running.

>>> inst.remove_inactive_containers("m1") Fatal error: local() encountered an error (return code 1) while executing 'docker inspect --format "{{ index .Config.Labels \"ai.clipper.model_container.model_version\"}}" ' ================================ Standard error ================================ "docker inspect" requires at least 1 argument(s). See 'docker inspect --help'. Usage: docker inspect [OPTIONS] NAME|ID [NAME|ID...] Return low-level information on Docker objects ================================================================================ Aborting.

Good catch. Fixed.

Corey-Zumar · 2017-06-02T14:15:37Z

clipper_admin/clipper_manager.py

-                subprocess.call("docker save -o /tmp/{fn}.tar {cn}".format(
-                    fn=saved_fname, cn=container_name))
-                tar_loc = "/tmp/{fn}.tar".format(fn=saved_fname)
                self._execute_put(tar_loc, tar_loc)


tar_loc is now undefined yet is being referenced. Why were these lines removed?

Oh weird. They were not supposed to be. Adding them back.

Corey-Zumar · 2017-06-02T14:16:58Z

examples/basic_query/example_client.py

 if __name__ == '__main__':
    host = "localhost"
-    clipper = cm.Clipper(host, check_for_docker=False)
+    clipper = Clipper(host, check_for_docker=False)


why don't we check for docker here?

Because this example doesn't start Clipper, it assumes there is already a Clipper instance running and model connected. We don't check for Docker so that the example works even if you aren't using Docker to run Clipper.

Got it! The key part is the docker-independent functionality.

jegonzal

@Corey-Zumar did an excellent job reviewing. I would approve pending his comments.

AmplabJenkins · 2017-06-02T16:40:26Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/361/
Test FAILed.

AmplabJenkins · 2017-06-02T16:58:19Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/362/
Test FAILed.

AmplabJenkins · 2017-06-02T17:38:17Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/364/
Test FAILed.

AmplabJenkins · 2017-06-02T19:07:36Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/365/
Test PASSed.

Corey-Zumar

LGTM. Let me know if I should hold off until #187 is merged. If not, we should create an issue reminding us to write a test for remove_inactive_containers.

AmplabJenkins · 2017-06-03T05:13:22Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/367/
Test PASSed.

…ainers

dcrankshaw · 2017-06-03T21:40:24Z

@Corey-Zumar this went through a big round of changes, so you'd better review again

feynmanliang · 2017-06-04T20:27:34Z

clipper_admin/clipper_manager.py

                     container_name,
-                     labels,
                     input_type,
+                     labels=[DEFAULT_LABEL],


What's the purpose of having all models get a "default" label? Can this be defaulted to []?

feynmanliang · 2017-06-04T20:32:26Z

clipper_admin/clipper_manager.py

+        # Get all Docker containers tagged as model containers
+        num_containers_removed = 0
+        with hide("output", "warnings", "running"):
+            containers = self._execute_root(


What is the reasoning for dropping into shell commands over using libraries? e.g. https://docker-py.readthedocs.io/en/stable/containers.html#docker.models.containers.ContainerCollection.list

Simplicity and wanting to use the same commands locally and remotely. As we refactor clipper_manager.py we should definitely look into the Python SDK.

Corey-Zumar

Just requesting that we rename an integration test file + a nit comment. Looks good in general

Corey-Zumar · 2017-06-04T19:56:31Z

bin/run_unittests.sh

  echo -e "\nRunning integration tests\n\n"
  cd $DIR
+  python ../integration-tests/clipper_manager_tests.py
  python ../integration-tests/light_load_all_functionality.py 2 3


Now that we have multiple integration tests, let's rename light_load_all_functionality to something more descriptive.

Corey-Zumar · 2017-06-04T20:32:51Z

src/frontends/src/query_frontend_tests.cpp

  std::string test_json_string =
-      "{\"uid\": 23, \"input\": \"hello world. This is a test string with "
+      "{\"input\": \"hello world. This is a test string with "
      "punctionation!@#$Y#;}#\"}";


Very nit: Typo in "punctuation".

This isn't a real word anyway. Leaving as is.

feynmanliang · 2017-06-04T20:35:35Z

integration-tests/clipper_manager_tests.py

 cur_dir = os.path.dirname(os.path.abspath(__file__))
 sys.path.insert(0, os.path.abspath('%s/../' % cur_dir))
-import clipper_manager
+from clipper_admin import clipper_manager


AFAICT clipper_manager is only used to access the Clipper class so we could directly import Clipper here

Ahh yeah good catch. This is a holdover from before the package structure existed.

feynmanliang · 2017-06-04T20:36:03Z

integration-tests/light_load_all_functionality.py

-sys.path.insert(0, os.path.abspath('%s/../clipper_admin/' % cur_dir))
-import clipper_manager as cm
+sys.path.insert(0, os.path.abspath('%s/../' % cur_dir))
+from clipper_admin import clipper_manager as cm


Ditto about importing Clipper directly

dcrankshaw · 2017-06-04T23:49:33Z

@Corey-Zumar @feynmanliang addressed your comments. @Corey-Zumar please merge once the tests pass.

Corey-Zumar

LGTM

AmplabJenkins · 2017-06-05T00:11:49Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/390/
Test FAILed.

AmplabJenkins · 2017-06-05T00:42:55Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/391/
Test FAILed.

dcrankshaw · 2017-06-05T00:54:50Z

jenkins test this please

AmplabJenkins · 2017-06-05T01:07:19Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/393/
Test FAILed.

AmplabJenkins · 2017-06-05T01:35:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/394/
Test FAILed.

AmplabJenkins · 2017-06-05T01:50:25Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/395/
Test FAILed.

AmplabJenkins · 2017-06-05T02:14:25Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/396/
Test PASSed.

dcrankshaw added status: needs review type: enhancement labels Jun 2, 2017

dcrankshaw self-assigned this Jun 2, 2017

dcrankshaw requested review from Corey-Zumar and jegonzal June 2, 2017 08:05

Corey-Zumar requested changes Jun 2, 2017

View reviewed changes

Corey-Zumar added status: needs revision and removed status: needs review labels Jun 2, 2017

jegonzal reviewed Jun 2, 2017

View reviewed changes

dcrankshaw added status: needs review and removed status: needs revision labels Jun 2, 2017

Corey-Zumar approved these changes Jun 3, 2017

View reviewed changes

Corey-Zumar added status: accepted and removed status: needs review labels Jun 3, 2017

dcrankshaw added 10 commits June 3, 2017 11:52

removed uid from prediction query schema

970f92c

added method to remove inactive containers

18584cc

add Clipper class to clipper_admin module

6964b85

Make model labels optional in clipper_admin

e2e54e5

make inspect_selection_policy private

ca5a9d5

added check for python3 and note about downloading docker images

cdbeb85

addressed review comments. Still need a test for remove_inactive_cont…

ee90331

…ainers

format code

06cd6ce

Fix unit tests

60bb9dd

fix integration test

5826a19

dcrankshaw added status: needs review and removed status: accepted labels Jun 3, 2017

dcrankshaw mentioned this pull request Jun 4, 2017

Adds support for deploying PySpark models #196

Merged

feynmanliang reviewed Jun 4, 2017

View reviewed changes

Corey-Zumar requested changes Jun 4, 2017

View reviewed changes

Corey-Zumar added status: needs revision and removed status: needs review labels Jun 4, 2017

feynmanliang reviewed Jun 4, 2017

View reviewed changes

dcrankshaw added 2 commits June 4, 2017 16:44

addressed review comments

872e0af

format code

f52dbe4

Corey-Zumar approved these changes Jun 4, 2017

View reviewed changes

Corey-Zumar added status: accepted and removed status: needs revision labels Jun 4, 2017

fixed check for missing container info

8d624db

debugging

f60fcff

fixed unittest script

acf9c11

added default label

ab0d0e2

format code

0bcd4ca

Corey-Zumar merged commit fc7e2be into ucbrise:develop Jun 5, 2017

Conversation

dcrankshaw commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jegonzal left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 2, 2017

Uh oh!

AmplabJenkins commented Jun 2, 2017

Uh oh!

AmplabJenkins commented Jun 2, 2017

Uh oh!

AmplabJenkins commented Jun 2, 2017

Uh oh!

Corey-Zumar left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jun 3, 2017

Uh oh!

dcrankshaw commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Corey-Zumar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dcrankshaw commented Jun 2, 2017 •

edited

Loading